SqlScript, SqlScriptQuery: Add support for disable variables parameter #1578
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1578 +/- ##
======================================
Coverage 98% 98%
======================================
Files 37 37
Lines 6074 6082 +8
======================================
+ Hits 5970 5978 +8
Misses 104 104 |
|
@Kreby Thank you for this! 😃 I might not have time to review this until the weekend due other obligations, but will get onto this as soon as I can! |
There was a problem hiding this comment.
Reviewed 12 of 12 files at r1.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @Kreby)
CHANGELOG.md, line 31 at r1 (raw file):
- Added the DisableVariables parameter ([issue #1422](https://github.com/dsccommunity/SqlServerDsc/issues/1422))
Can we add a full stop (.) at the en of the sentence to align with the other entries.
CHANGELOG.md, line 33 at r1 (raw file):
- Added the DisableVariables parameter ([issue #1422](https://github.com/dsccommunity/SqlServerDsc/issues/1422))
Can we add a full stop (.) at the en of the sentence to align with the other entries.
tests/Integration/DSC_SqlScript.config.ps1, line 32 at r1 (raw file):
Database3Name = '$(DatabaseName)'
This is not used, right? We can remove it?
tests/Integration/DSC_SqlScript.config.ps1, line 261 at r1 (raw file):
Quoted 4 lines of code…
<# .SYNOPSIS Runs the SQL script with variables disabled. #>
Can we add a .NOTES section with some of the text you wrote in the PR description about that it will create a database with the name $(DatabaseName) so next contributor more easily understand the logic. 🙂
tests/Integration/DSC_SqlScriptQuery.config.ps1, line 30 at r1 (raw file):
Database3Name = '$(DatabaseName)'
This is not used, right? We can remove it?
tests/Integration/DSC_SqlScriptQuery.config.ps1, line 119 at r1 (raw file):
Quoted 4 lines of code…
<# .SYNOPSIS Runs the SQL query with variables disabled. #>
Same comment about a .NOTES section as above.
|
Great work on this! Just minor changes then this is good to go. 😃 |
There was a problem hiding this comment.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @Kreby)
tests/Integration/DSC_SqlScript.config.ps1, line 32 at r1 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Database3Name = '$(DatabaseName)'This is not used, right? We can remove it?
It was used in the integration tests, I missed that. Removed the comment. 🙂
tests/Integration/DSC_SqlScriptQuery.config.ps1, line 30 at r1 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Database3Name = '$(DatabaseName)'This is not used, right? We can remove it?
It was used in the integration tests, I missed that. Removed the comment. 🙂
|
@johlju thanks for the kind words/support. I'm glad to help out. I've not used reviewable before so I was a little disoriented on my first look 😵. I wasn't quite sure how to respond appropriately to the individual comments with the few minutes I had. I'll spend sometime later today or tomorrow to figure that out, make the requested changes, and update my branch. Thanks! |
johlju
added
waiting for code fix
|
Yes, Reviewable can take a while to get used to (it did for me) but is awesome once you are. 🙂 Basically you just write Done as a comment when you have sent in a change for a comment, or write a comment when you want to discuss something. Once you answered the comments you like you hit the big green publish button at the top, that will publish all comments back to the PR as one big PR comment. When you write Done for a comment then I can "Resolve" them in Reviewable. |
…into add-disable-variables-for-sqlscript-sqlscriptquery
…into add-disable-variables-for-sqlscript-sqlscriptquery
There was a problem hiding this comment.
Reviewable status: 9 of 12 files reviewed, 4 unresolved discussions (waiting on @johlju)
CHANGELOG.md, line 31 at r1 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
- Added the DisableVariables parameter ([issue #1422](https://github.com/dsccommunity/SqlServerDsc/issues/1422))Can we add a full stop (
.) at the en of the sentence to align with the other entries.
Done.
CHANGELOG.md, line 33 at r1 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
- Added the DisableVariables parameter ([issue #1422](https://github.com/dsccommunity/SqlServerDsc/issues/1422))Can we add a full stop (
.) at the en of the sentence to align with the other entries.
Done.
tests/Integration/DSC_SqlScript.config.ps1, line 261 at r1 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
<# .SYNOPSIS Runs the SQL script with variables disabled. #>Can we add a
.NOTESsection with some of the text you wrote in the PR description about that it will create a database with the name$(DatabaseName)so next contributor more easily understand the logic. 🙂
I've updated the comment based help with details.
tests/Integration/DSC_SqlScriptQuery.config.ps1, line 119 at r1 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
<# .SYNOPSIS Runs the SQL query with variables disabled. #>Same comment about a
.NOTESsection as above.
I've updated the comment based help with details.
There was a problem hiding this comment.
Reviewed 3 of 3 files at r2.
Reviewable status:complete! all files reviewed, all discussions resolved
|
@Kreby thank you for this! I merged it and will do a full release a bit later today. 🙂 |
johlju
removed
the
waiting for code fix
Pull Request (PR) description
This PR adds support for the DisableVariables parameter from the Invoke-SqlCmd cmdlet. It should hopefully be relatively straight forward. The only thing I'd point out is in the integration tests used. I was able to keep the same SQL queries as the other tests. Just note that when the test is run with the DisableVariables parameter it will create the database and give it the name of the variable used
Despite being a bit odd looking it is a valid database name and when matched, just like the other tests, it will pass. It does also require, as mentioned in a comment, that the database is removed after the test runs to ensure the subsequent test succeeds as well. This is because the same variable name is used in both resource configuration data variables. I think it should suffice but please let me know if it doesn't.
This Pull Request (PR) fixes the following issues
Task list
file CHANGELOG.md. Entry should say what was changed and how that
affects users (if applicable), and reference the issue being resolved
(if applicable).
and comment-based help.
This change is